feat: query max amountIn based on max ticks willing to traverse #5889
Conversation
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
drive by lint (unchecked error)
| @@ -808,3 +808,120 @@ func edgeCaseInequalityBasedOnSwapStrategy(isZeroForOne bool, nextInitializedTic | |||
| } | |||
| return nextInitializedTickSqrtPrice.LT(computedSqrtPrice) | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Note to reviewer, ComputeMaxInAmtGivenMaxTicksCrossed follows nearly the same logic as calcIn/Out, but tracks the ticks traversed, does not track the spread or uptime accums, and does not save changes to state.
|
This interface looks great to me and is all that Protorev will need to bound properly, thanks for this! |
| ctx sdk.Context, | ||
| poolId uint64, | ||
| tokenInDenom string, | ||
| maxTicksCrossed uint64, |
There was a problem hiding this comment.
Just curious: since tick liquidity is subject to change, how is protorev going to bound the max tick a swap is going to cross? Is it something like X swap can cross upto 100 ticks given the gas to cross that 100 ticks is <50M
I originally thought this function was going to be given X amt estimate how many ticks it will cross, but if this works then it's all g.
There was a problem hiding this comment.
@stackman27 I previously thought we wanted "Given X amount, how many ticks will it cross?" but since Protorev needs to bound itself from consuming too much gas and compute time, and it seems to me liquidity doesn't really impact that, but number of ticks crossed does, so it cares more about the ticks crossed.
If a method was made that given X amount how many ticks will it cross, and we put in $1k to swap in and it says it'll cross 1000 ticks, that would be too much and then the next action would be "okay how do I reduce the number of ticks crossed to something acceptable", so it feels like just querying directly for the acceptable number of ticks crossed and always using that amountIn bound is better than continuously trying in different amounts to identify what amount passes an acceptable amount of ticks.
There was a problem hiding this comment.
i see one last question, so in the current implementation how are you going to decide the maxTickCrossed value? are you going to predetermine that 50M or 100M gas can cross X amt of ticks and generate routes for swap?
There was a problem hiding this comment.
I think the answer to this is crossing ticks should cost a fixed amt of gas so yes, your description should be accurate (but admittedly I just made this based of Skip's design requirements and treated the actual use of the API as a black box)
There was a problem hiding this comment.
@stackman27 @czarcas7ic Yes exactly. The plan is to make X number of ticks be a changeable variable (similar to pool points), and change that depending on how many CL pools we want to be able to use in backrunning.
That is, if many new CL pools launch and almost all swaps are on CL pools, then we'd want to decrease the max number of ticks crossed per pool so that we can check 2-4 routes per swap without running out of the upperGasLimit, if there are still more balancer swaps than CL, and our routes mainly consist of balancer pool, then we can increase X to cross more ticks per CL pool since there are less of them, etc.
There was a problem hiding this comment.
sounds good, would love to review the PR when its ready too
x/concentrated-liquidity/swaps.go
Outdated
| } | ||
|
|
||
| // Initialize swap state | ||
| swapState := newSwapState(types.MaxSpotPrice.RoundInt(), p, swapStrategy) |
There was a problem hiding this comment.
Why do we use types.MaxSpotPrice? Do we decrement it until maxTicksCrossed ?
There was a problem hiding this comment.
Yeah we basically never want the reason we stop an estimation to be due to a lack of funds, it should only ever strictly be due to the limit imposed by amt of ticks willing to traverse.
There was a problem hiding this comment.
in that case would TotalPoolLiquidity not be a good value to use?
There was a problem hiding this comment.
Yeah I think that makes sense too, I can change it to that
|
@NotJeremyLiu Am merging this now, just pinging for awareness |
…sis-labs#5889) * max amountIn based on max ticks willing to trav * update changelog * clean up * use cache context in the query * change amount in from max spot price to pool bal * change to tokenoutdenom
Closes: #5884
What is the purpose of the change
This PR provides a new API (
ComputeMaxInAmtGivenMaxTicksCrossed), which is used to determine the max amount a token cab be swapped in within a CL pool given a max number of ticks that can be traversed.Testing and Verifying
TestComputeMaxInAmtGivenMaxTicksCrossedadded.Documentation and Release Note
Unreleasedsection ofCHANGELOG.md?Where is the change documented?
x/{module}/README.md)